Skip to content
This repository was archived by the owner on Dec 25, 2019. It is now read-only.

sqlparser changes to support timestamptz#21

Merged
ruchirK merged 1 commit intoMaterializeInc:masterfrom
ruchirK:timestamptz-2
Nov 18, 2019
Merged

sqlparser changes to support timestamptz#21
ruchirK merged 1 commit intoMaterializeInc:masterfrom
ruchirK:timestamptz-2

Conversation

@ruchirK
Copy link
Copy Markdown
Contributor

@ruchirK ruchirK commented Nov 13, 2019

No description provided.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Nov 13, 2019

CLA assistant check
All committers have signed the CLA.

@quodlibetor quodlibetor self-requested a review November 13, 2019 20:29
Copy link
Copy Markdown
Contributor

@quodlibetor quodlibetor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to comment some nits on a wip, I was reading for understanding and threw them in.

Comment thread src/parser/datetime.rs Outdated
Comment thread src/parser/datetime.rs Outdated
Comment thread src/parser/datetime.rs Outdated
Comment thread src/parser/datetime.rs Outdated
Comment thread src/parser/datetime.rs
Comment thread src/ast/value/datetime.rs
@ruchirK ruchirK changed the title [wip / don't review] sqlparser changes to support timestamptz sqlparser changes to support timestamptz Nov 14, 2019
Comment thread src/parser.rs
Comment thread src/parser.rs Outdated
Comment thread src/parser.rs Outdated
Comment thread src/parser/datetime.rs
Comment thread src/parser/datetime.rs Outdated
Comment thread src/parser/datetime.rs Outdated
Comment thread src/parser/datetime.rs Outdated
Comment thread src/parser/datetime.rs Outdated
Comment thread src/parser/datetime.rs Outdated
Comment thread tests/sqlparser_common.rs Outdated
@ruchirK ruchirK force-pushed the timestamptz-2 branch 3 times, most recently from 7087fe2 to f54cc7d Compare November 18, 2019 16:26
Copy link
Copy Markdown
Contributor

@quodlibetor quodlibetor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two nit comments, and one request for clarification either via a comment or a refactoring.

Comment thread src/parser/datetime.rs Outdated
Comment thread src/parser/datetime.rs Outdated
Comment thread src/parser/datetime.rs
Comment on lines +268 to +275
(_, _) => {
is_positive = true;
hour_offset = None;
minute_offset = None;
break;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though I recognize that this is obviously working, I don't think that I follow it. Does it unconditionally go through every format string, even if the first one matches? That would explain why this needs to be at the end instead of the beginning. Would it be easier to reason about this if this inner match statement was in an inner function that can signal success by returning a success/fail value?

The reason this feels worth doing is that I believe that the entire function currently depends on the order of items in that format list plus the order of items in this match, and is therefore possibly pretty fragile in the face of refactorings. The tests will prevent people from introducing incorrect code, but I could see trying to interact with this code requiring a lot of understanding.

If you don't feel like experimenting with a refactoring, could you add a comment at the top of the possible formats loop (line 217) explaining the ways that these parts all fit together?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the intention was to match on the first format that validly parses (the structure is a doubly nested for loop so the break statement is there when we've detected a mismatch between the format and the actual list of tokens and lets the control flow go to the next one), with the understanding that all of the potential formats with numbers are going to have numbers in HH MM order, so if the first number is >= 24, then we can just fail everything because we know it will never pass any format (as opposed to a token mismatch which could pass a different format)

I'll refactor a bit and add comments

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

Copy link
Copy Markdown
Contributor

@quodlibetor quodlibetor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥 :shipit: looks great!

@ruchirK ruchirK merged commit f5812b4 into MaterializeInc:master Nov 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants